Fix: VE.Direct refactor issues from #505 #516
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the changed return statement was supposed to return a shared_ptr to a new and valid MPPT data struct as a fallback. however, it did return a new shared_ptr that was initialized to nullptr.
this change makes the call to VictronMppt.getData() obsolete, which in turn will therefore not cause an error message on the console if VE.Direct (MPPT) is not enabled. this change also takes care that once multiple VE.Direct MPPT charge controllers are supported, the sums of the respective total values are used in the web app totals.
This was referenced Oct 22, 2023
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The refactor in #505 was not tested with VE.Direct turned off. Sorry for that.
If VE.Direct is turned off, the web API implementation still adds totals of the MPPT to be displayed by the web app on the top. It used
VictronMppt.getData(), which did print an error message on the console, but did not return the proper fallback data structure. It returned astd::shared_ptrpointing tonullptrrather than one pointing to a default-initialized MPPT data struct, which was my intention.The fix is simple, see the first commit. However, the error message is still printed on the console, since the web API implementation adds the totals unconditionally. Adding them only if VE.Direct is enabled is not working out, since those values are defined to be part of the respective typescript type definition, so the web app crashes if those values are missing. Also, they would only add the values of the first MPPT controller. In the future, these totals should be the respective sum over all MPPT controller of these totals. Hence the second commit implements the respective methods. Using them instead of
VictronMppt.getData()also gets rid of the error message in the log in case VE.Direct is disabled.All other uses of
VictronMppt.getData()are now safe, as was originally intended. However, (manual) access toapi/vedirectlivedata/statuswill still print the aforementioned error message to the log. Changing this is not really possible (without further refactoring of the web app). The API endpoint should not respond with default-initialized values if the data is invalid or VE.Direct is not enabled. However, this was previously the case and will be the case now as well. This should be fixed once the liveview adds support for multiple MPPT charge controllers: The API endpoint will return a list of MPPT charge controller values, which will be empty if VE.Direct is disabled or the data is invalid.Note that the aforementioned error message will not appear when accessing the live view, as the web app will not connect to the VE.Direct live view API endpoint if VE.Direct is disabled.